Skip to content

Regular validator updates MTA-STS-dependent policies #217

Closed
sydneyli wants to merge 5 commits intomasterfrom
mta-sts-upgrade
Closed

Regular validator updates MTA-STS-dependent policies #217
sydneyli wants to merge 5 commits intomasterfrom
mta-sts-upgrade

Conversation

@sydneyli
Copy link
Contributor

For each MTA-STS-dependent policy, checks MTA-STS policy
against internal DB hostname store, and performs an update if they don't match.

Depends on #216 ~

For each MTA-STS-dependent policy, checks MTA-STS policy
against internal DB hostname store.
db/sqldb.go Outdated
"ON CONFLICT (domain) DO UPDATE SET status=$5",
domain.Name, domain.Email, strings.Join(domain.MXs[:], ","),
models.StateUnvalidated, domain.State, domain.QueueWeeks)
models.StateUnvalidated, domain.State, domain.QueueWeeks, domain.MTASTSMode == "on")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we had discussed converting this to a boolean field called MTASTSMode. Do you need a string to store the mode later on or is a boolean still okay?

models/domain.go Outdated
// We should only trust MTA-STS info from a successful MTA-STS check.
if scan.Data.MTASTSResult != nil && scan.SupportsMTASTS() {
d.MTASTSMode = scan.Data.MTASTSResult.Mode
if d.MTASTSMode == "on" && scan.Data.MTASTSResult != nil && scan.SupportsMTASTS() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scan.Data.MTASTSResult != nil is built into scan.SupportsMATSTS()


// UpdateDomainPolicy allows us to update the internal data about a particular domain.
func (db *SQLDatabase) UpdateDomainPolicy(domain models.Domain) error {
_, err := db.conn.Exec("UPDATE domains SET data=$2, status=$3 WHERE domain=$1 AND mta_sts=TRUE",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will update the policy and status for all matching domains - there could but multiple (domain, status) pairs returned. Seems like updating status for all of them could cause a collision.

policy, err := l.Get(domain)
if err != nil {
return []string{}, err
return models.Domain{}, err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this reuse of the domain model!

@vbrown608 vbrown608 self-assigned this Apr 30, 2019
@sydneyli sydneyli force-pushed the mta-sts-upgrade branch from d479947 to 7ebf3cd Compare May 6, 2019 22:26
@sydneyli
Copy link
Contributor Author

sydneyli commented May 6, 2019

Thanks for the review @vbrown608 ! I just merged with upstream and am fixing a couple of things. Will ping this thread once I think it's ready for review :)

@sydneyli
Copy link
Contributor Author

I think this will have to depend on the cleaner abstractions in #245 to do in a way that doesn't cause us more headache. Closing for now, going to port code to sit on top of that branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants